Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: HTTP/429 when requesting authors information, resolves #1570 #2188

Merged
merged 11 commits into from
Jun 9, 2024

Conversation

jfrazx
Copy link
Contributor

@jfrazx jfrazx commented Oct 6, 2023

  • Utilize a rate limiter when requesting matching author information
  • Ensure Audnexus class is a singleton

@advplyr
Copy link
Owner

advplyr commented Oct 14, 2023

I put in a limiter for the login using express-rate-limit but added in the server/libs/ folder here https://github.com/advplyr/audiobookshelf/blob/master/server/libs/expressRateLimit/index.js

Is this a library we could use instead? If we can use express-rate-limit then we can pull it out of the libs folder and add it to the package.json instead.

The limiter library adds an additional dependency just-performance that seems unnecessary at a quick glance.

@jfrazx
Copy link
Contributor Author

jfrazx commented Oct 15, 2023

I don't think express-rate-limit will work. Being express middleware it expects to receive request and response objects. The use case of each is different. express-rate-limit throttles incoming requests while limiter intends to assist in managing outgoing requests.

@nichwall
Copy link
Contributor

Could the throttleIt package work instead as a wrapper around existing functions?

https://github.com/sindresorhus/throttleit/blob/main/index.js

@jfrazx
Copy link
Contributor Author

jfrazx commented Jan 23, 2024

I don't think so. It appears throttleIt is for synchronous functions. But, the same author does have an async throttling package: p-throttle. I'll swap out limiter and see how well it works.

@jfrazx
Copy link
Contributor Author

jfrazx commented Jan 26, 2024

@advplyr Any other concerns with this PR?

server/utils/index.js Fixed Show fixed Hide fixed
@advplyr
Copy link
Owner

advplyr commented Jun 9, 2024

This is working well in my tests. Thanks!

@advplyr advplyr merged commit fcd74ae into advplyr:master Jun 9, 2024
4 checks passed
@jfrazx jfrazx deleted the fix/match-authors-429 branch June 10, 2024 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants